-
Notifications
You must be signed in to change notification settings - Fork 4.2k
File-based programs live directive diagnostics #80575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
File-based programs live directive diagnostics #80575
Conversation
src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
RikkiGibson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitting comments
src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/CSharp/Analyzers/FileBasedPrograms/AppDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
6233d9b to
e774d7a
Compare
|
Opened dotnet/sdk#51373. There are still a number of issues on both sdk and roslyn side when consuming this package. |
| <!-- dotnet/arcade-services dependencies --> | ||
| <MicrosoftDotNetDarcLibPackageVersion>1.1.0-beta.25503.1</MicrosoftDotNetDarcLibPackageVersion> | ||
| <!-- dotnet/sdk dependencies --> | ||
| <MicrosoftDotNetFileBasedProgramsPackageVersion>10.0.200-preview.0.25556.104</MicrosoftDotNetFileBasedProgramsPackageVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably setup a subscription so that we get pushed updates for this package automatically, but, want to start by just taking the version that was pushed to the feed yesterday.
src/Features/CSharp/Portable/Diagnostics/Analyzers/FileBasedPrograms/ExternalHelpers.cs
Show resolved
Hide resolved
src/Features/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.Features.csproj
Outdated
Show resolved
Hide resolved
...arp/Portable/Diagnostics/Analyzers/FileBasedPrograms/FileLevelDirectiveDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
...arp/Portable/Diagnostics/Analyzers/FileBasedPrograms/FileLevelDirectiveDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ripping out all the bits that were reporting "build only diagnostics" for #: directives.
...arp/Portable/Diagnostics/Analyzers/FileBasedPrograms/FileLevelDirectiveDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
...arp/Portable/Diagnostics/Analyzers/FileBasedPrograms/FileLevelDirectiveDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
|
|
||
| // App directives are only valid when they appear before the first C# token | ||
| var rootLeadingTrivia = root.GetLeadingTrivia(); | ||
| var diagnosticBag = DiagnosticBag.Collect(out var diagnosticsBuilder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this type is unfamiliar to me. need to figure out what that is about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| var rootLeadingTrivia = root.GetLeadingTrivia(); | ||
| var diagnosticBag = DiagnosticBag.Collect(out var diagnosticsBuilder); | ||
| FileLevelDirectiveHelpers.FindLeadingDirectives( | ||
| new SourceFile(tree.FilePath, tree.GetText(context.CancellationToken)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need the entire text? Is it redundant with the leading trivia you are passing in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently it is used here:
I believe we can get rid of this. I will add this to a tracking issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to dotnet/sdk#51487
| return Diagnostic.Create( | ||
| Rule, | ||
| location: Location.Create(syntaxTree, simpleDiagnostic.Location.TextSpan), | ||
| simpleDiagnostic.Message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the analyzer is included under this path, because we only expect/want it to run in the editor. Running it on the command line would just be redundant with what dotnet build app.cs already does.
It wouldn't cause a problem if it did somehow run on the command line, though, because in general, in a scenario where this analyzer would report an error, dotnet build app.cs would have reported an error at an earlier stage, and the compiler wouldn't run.
…ikkiGibson/roslyn into fbp-live-directive-diagnostics
|
Note that one challenge with this work is: it basically takes a full day to propagate any changes in the source package over to roslyn. That really is how long it takes for an sdk change to get merged, flow into dotnet/dotnet, for unified build to run and the package to get published to the proper feed. Obviously local package builds can be used for local runs, but, I'm talking about availability of the package in such a form that we can actually merge to roslyn main. |
|
I am seeing several new public API analyzer errors under https://github.com/dotnet/roslyn/tree/b43a5601a13cb4ca3b95c66f9d13dc0448817e04/src/Features/CSharp/Portable/ExternalAccess/Pythia. It's not clear to me why my change is causing this.
Possibly, use of the FBP package is causing a PublicApiAnalyzer version to get bumped in MS.CA.CS.Features, or something? I doubt that because > dotnet nuget why .\src\Features\CSharp\Portable\ Microsoft.CodeAnalysis.PublicApiAnalyzers
Project 'Microsoft.CodeAnalysis.CSharp.Features' has the following dependency graph(s) for 'Microsoft.CodeAnalysis.PublicApiAnalyzers':
[net8.0]
[net9.0]
[netstandard2.0]
│
└─ Roslyn.Diagnostics.Analyzers (v3.11.0-beta1.24081.1)
└─
Microsoft.CodeAnalysis.PublicApiAnalyzers (v3.11.0-beta1.24081.1)I am planning to address by suppressing the errors under |
| { | ||
| public const string DiagnosticId = "FileBasedPrograms"; | ||
|
|
||
| private static readonly DiagnosticDescriptor Rule = CreateDescriptor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should private static fields follow this convention?
| private static readonly DiagnosticDescriptor Rule = CreateDescriptor( | |
| private static readonly DiagnosticDescriptor s_rule = CreateDescriptor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusting the name to make it look more similar to other call sites of CreateDescriptor.
src/EditorFeatures/Test/Diagnostics/IDEDiagnosticIDConfigurationTests.cs
Outdated
Show resolved
Hide resolved
| private async Task<(string VirtualProjectXml, ImmutableArray<SimpleDiagnostic> Diagnostics)?> GetVirtualProjectContentImplAsync(string documentFilePath, ILogger logger, CancellationToken cancellationToken) | ||
| { | ||
| var workingDirectory = Path.GetDirectoryName(documentFilePath); | ||
| var process = dotnetCliHelper.Run(["run-api"], workingDirectory, shouldLocalizeOutput: true, keepStandardInputOpen: true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we still need to call run-api?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would want to add a method to source package for getting a virtual project and use it here before getting rid of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so we only wanted the source package for live diagnostics to be visible even without saving the file (which would not be easily possible with run-api). For other things, run-api still works fine. Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I think it's a good idea to expand the source package, and use it to replace the task being done by this method, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to using source package instead of calling the run-api.
| Namespace="Microsoft.DotNet.FileBasedPrograms" /> | ||
| </ItemGroup> | ||
| <PropertyGroup> | ||
| <DefineConstants>$(DefineConstants);FILE_BASED_PROGRAMS_SOURCE_PACKAGE_GRACEFUL_EXCEPTION</DefineConstants> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of GracefulException, and associated compilation symbol, should be removed: dotnet/sdk#51373 (comment)
This symbol causes the source package to declare type GracefulException which is currently used in the FileBasedPrograms implementation. We want to remove the use of this type from the source package once we have the time to do so.
| private async Task<(string VirtualProjectXml, ImmutableArray<SimpleDiagnostic> Diagnostics)?> GetVirtualProjectContentImplAsync(string documentFilePath, ILogger logger, CancellationToken cancellationToken) | ||
| { | ||
| var workingDirectory = Path.GetDirectoryName(documentFilePath); | ||
| var process = dotnetCliHelper.Run(["run-api"], workingDirectory, shouldLocalizeOutput: true, keepStandardInputOpen: true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to using source package instead of calling the run-api.
Closes #80006
Depends on changes in dotnet/sdk#51373